Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(pnpm): url and git package specifiers #1754

Closed
wants to merge 2 commits into from
Closed

fix(pnpm): url and git package specifiers #1754

wants to merge 2 commits into from

Conversation

jbedard
Copy link
Member

@jbedard jbedard commented May 23, 2024

Ref #1753

Changes are visible to end-users: no

Test plan

  • Covered by existing test cases
  • New test cases added

@jbedard jbedard requested a review from gregmagolan May 23, 2024 18:11
@jbedard jbedard force-pushed the pnpm-normalize-edge-cases branch from 406038d to 4d3312e Compare May 23, 2024 18:13
@aspect-workflows Aspect Workflows
Copy link

aspect-workflows bot commented May 23, 2024 •
edited
Loading

Test

All tests were cache hits

191 tests (100.0%) were fully cached saving 52s.

Test

e2e/bzlmod

All tests were cache hits

4 tests (100.0%) were fully cached saving 467ms.

Test

e2e/gyp_no_install_script

All tests were cache hits

2 tests (100.0%) were fully cached saving 693ms.

Test

e2e/js_image_oci

All tests were cache hits

1 test (100.0%) was fully cached saving 9s.

Test

e2e/npm_link_package

All tests were cache hits

2 tests (100.0%) were fully cached saving 1s.

Test

e2e/npm_link_package-esm

All tests were cache hits

2 tests (100.0%) were fully cached saving 923ms.

Test

e2e/npm_translate_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 73ms.

Test

e2e/npm_translate_lock_empty

All tests were cache hits

1 test (100.0%) was fully cached saving 73ms.

Test

e2e/npm_translate_lock_multi

All tests were cache hits

2 tests (100.0%) were fully cached saving 271ms.

Test

e2e/npm_translate_lock_partial_clone

All tests were cache hits

1 test (100.0%) was fully cached saving 117ms.

Test

e2e/npm_translate_lock_subdir_patch

All tests were cache hits

1 test (100.0%) was fully cached saving 248ms.

Test

e2e/npm_translate_package_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 73ms.

Test

e2e/npm_translate_yarn_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 73ms.

Test

e2e/package_json_module

All tests were cache hits

1 test (100.0%) was fully cached saving 520ms.

Test

e2e/pnpm_lockfiles

⚠️ Buildkite build #4192 failed.

Failed tests (3) 
//v54:repos_0_test [k8-fastbuild]536ms
//v60:repos_0_test [k8-fastbuild]686ms
//v61:repos_0_test [k8-fastbuild]529ms

💡 To reproduce the test failures, run

bazel test //v54:repos_0_test //v60:repos_0_test //v61:repos_0_test

Test

e2e/pnpm_workspace

All tests were cache hits

8 tests (100.0%) were fully cached saving 3s.

Test

e2e/pnpm_workspace_rerooted

All tests were cache hits

6 tests (100.0%) were fully cached saving 2s.

Test

e2e/repo_mapping

All tests were cache hits

2 tests (100.0%) were fully cached saving 451ms.

Test

e2e/rules_foo

All tests were cache hits

2 tests (100.0%) were fully cached saving 605ms.

Test

e2e/vendored_node

All tests were cache hits

1 test (100.0%) was fully cached saving 187ms.

Buildifier      Format

@jbedard jbedard mentioned this pull request May 23, 2024
Closed
@jbedard jbedard force-pushed the pnpm-normalize-edge-cases branch 2 times, most recently from 9acbca6 to 5695edf Compare May 23, 2024 18:50
@jbedard jbedard marked this pull request as draft May 23, 2024 19:25
@jbedard jbedard force-pushed the pnpm-normalize-edge-cases branch 3 times, most recently from b435d45 to 9c7f5b9 Compare May 27, 2024 22:31
@jbedard jbedard force-pushed the pnpm-normalize-edge-cases branch 4 times, most recently from 1088489 to 1750891 Compare May 28, 2024 00:14
@jbedard jbedard force-pushed the pnpm-normalize-edge-cases branch from 1750891 to eebcde1 Compare May 28, 2024 00:18
gregmagolan
gregmagolan reviewed May 28, 2024
View reviewed changes
e2e/pnpm_lockfiles/base/package.json
@@ -3,6 +3,9 @@
"private": true,
"dependencies": {
"@aspect-test/a": "^5.0.2",
"debug": "ngokevin/debug#9742c5f383a6f8046241920156236ade8ec30d53",
"hello": "https://gitpkg.vercel.app/EqualMa/gitpkg-hello/packages/hello",
"jsonify": "https://github.com/aspect-build/test-packages/releases/download/0.0.0/@foo-jsonify-0.0.0.tgz",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there more from npm/private/test/package.json that should be pulled into e2e/pnpm_lockfiles?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah there probably are. I need to get this PR actually working before I add much more though.

The debug and hello ones are unique cases (odd git syntax, https syntax). The jsonify one is also plain http but has a more interesting URL which requires escaping.

@jbedard
Copy link
Member Author

jbedard commented May 28, 2024

What was originally intended to be a prefactor for this has turned into the fix: #1765

@jbedard jbedard closed this May 28, 2024
@jbedard jbedard deleted the pnpm-normalize-edge-cases branch May 28, 2024 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregmagolan gregmagolan gregmagolan left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jbedard @gregmagolan